Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DH-227] apply changes from running pre-commit run --all-files #6386

Merged
merged 4 commits into from
Oct 18, 2024
Merged

[DH-227] apply changes from running pre-commit run --all-files #6386

merged 4 commits into from
Oct 18, 2024

Conversation

shaneknapp
Copy link
Contributor

<3 to massive PRs

@shaneknapp shaneknapp changed the title [DH-227] Fix pre commit stuff [DH-227] apply changes from running pre-commit run --all-files Oct 18, 2024
ryanlovett
ryanlovett approved these changes Oct 18, 2024
View reviewed changes
Copy link
Collaborator

@ryanlovett ryanlovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything obviously wrong in the code changes. I think it can be merged as is.

Another way would be to separate the changes into different PRs, e.g. one for line endings, one for imports, etc. That would just be to make things easier to review. But again, it seems fine.

@shaneknapp
Copy link
Contributor Author

Another way would be to separate the changes into different PRs, e.g. one for line endings, one for imports, etc. That would just be to make things easier to review. But again, it seems fine.

yeah, i agree but given how much time i've already dedicated to this i'm down to just do it this way... :)

balajialg
balajialg approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@balajialg balajialg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc changes LGTM!

felder
felder requested changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@felder felder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version lock in requirements.txt and dev-requirements.txt? Also scripts/infra-packages/requirements.txt has some interesting re-ordering, but we can probably just get rid of it.

@shaneknapp
Copy link
Contributor Author

@felder feel free to merge if you're happy w/the changes to requirements.txt and dev-requirements.txt

@felder felder merged commit f727517 into berkeley-dsep-infra:staging Oct 18, 2024
2 checks passed
@shaneknapp shaneknapp deleted the fix-pre-commit-stuff branch October 18, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felder felder felder requested changes

@ryanlovett ryanlovett ryanlovett approved these changes

@balajialg balajialg balajialg approved these changes

@gmerritt gmerritt Awaiting requested review from gmerritt

Assignees
No one assigned
Labels
bash configuration documentation Issues around adding and modifying docs grafana hub: a11y hub: astro hub: biology hub: cee hub: data100 hub: data101 hub: data102 hub: datahub hub: dev hub: dlab hub: edx hub: eecs hub: gradebook hub: highschool hub: ischool hub: julia hub: logodev hub: nature hub: prob140 hub: publichealth hub: r hub: shiny hub: stat20 hub: stat159 hub: ugr01 hub: workshop hub-images images issue-templates node-placeholder-deployment node-placeholder-scaler python support-deployment
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shaneknapp @felder @balajialg @ryanlovett